Skip to content

Rename MCO bootstrap presubmit and set it to always run #23020

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
openshift-merge-robot merged 2 commits into from
Nov 11, 2021
Merged

Rename MCO bootstrap presubmit and set it to always run #23020

openshift-merge-robot merged 2 commits into from
Nov 11, 2021

Conversation

@JoelSpeed
Copy link
Contributor

@JoelSpeed JoelSpeed commented Oct 26, 2021

Reverts #22940

Now that the PR to introduce the test is merged, we should make sure we are gathering the output on future PRs.

CC @kikisdeliveryservice @sinnykumari @yuqi-zhang

@kikisdeliveryservice
Copy link
Contributor

kikisdeliveryservice commented Oct 26, 2021

Given that we have 18 checks running on every PR - I'd much rather we get some runs in manually as opposed to having this auto-run on every PR and adding another.

Adding a hold to collect some runs and evaluate the status of the test.

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 26, 2021
@kikisdeliveryservice
Copy link
Contributor

kikisdeliveryservice commented Oct 26, 2021

link to runs in MCO repo: https://prow.ci.openshift.org/job-history/gs/origin-ci-test/pr-logs/directory/pull-ci-openshift-machine-config-operator-master-bootstrap-e2e

@sinnykumari
Copy link
Contributor

sinnykumari commented Oct 27, 2021

I know this is not the right place to bring this, but when I was manually typing to run this test on a PR I realized that test name is not consistent with others in our repo. e2e-bootstrap is more consistent than bootstrap-e2e. If others feel same way, we can rename the test to e2e-bootstrap.

@yuqi-zhang
Copy link
Contributor

yuqi-zhang commented Oct 27, 2021

Adding a hold to collect some runs and evaluate the status of the test.

I think this particular test should be pretty safe. It is not a e2e test in the sense that it actually has a running fully-formed cluster, so the chances of it being flaky (like platform tests) should be much lower.

If others feel same way, we can rename the test to e2e-bootstrap.

On the same vein, maybe this is better framed as a "bootstrap unit test"?

@JoelSpeed
Copy link
Contributor Author

JoelSpeed commented Oct 27, 2021

I think this particular test should be pretty safe. It is not a e2e test in the sense that it actually has a running fully-formed cluster, so the chances of it being flaky (like platform tests) should be much lower.

Agreed, I don't think there's really a need to not have this run, as it is more of a unit test and should not be flaky.

On the same vein, maybe this is better framed as a "bootstrap unit test"?

This makes a lot of sense

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 11, 2021
@JoelSpeed JoelSpeed force-pushed the revert-22940-bootstrap-e2e-dontalwaysrun branch from 16e8c88 to 07f7798 Compare November 11, 2021 11:40
@openshift-ci openshift-ci bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 11, 2021
@sinnykumari
Copy link
Contributor

sinnykumari commented Nov 11, 2021

Thanks Joel for updating the test name.
Can you also update subject of this PR to reflect both commit changes? Otherwise, LGTM
Will let other folks take a look as well in case there is any last minute change.

@JoelSpeed JoelSpeed changed the title Revert "mco: update bootstrap-e2e to always_run:false" Rename MCO bootstrap presubmit and set it to always run Nov 11, 2021
kikisdeliveryservice
kikisdeliveryservice approved these changes Nov 11, 2021
View reviewed changes
Copy link
Contributor

@kikisdeliveryservice kikisdeliveryservice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 11, 2021
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 11, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 11, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed, kikisdeliveryservice

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 11, 2021
@openshift-merge-robot openshift-merge-robot merged commit 74c862e into openshift:master Nov 11, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 11, 2021

@JoelSpeed: Updated the following 4 configmaps:

  • ci-operator-master-configs configmap in namespace ci at cluster app.ci using the following files:
    • key openshift-machine-config-operator-master.yaml using file ci-operator/config/openshift/machine-config-operator/openshift-machine-config-operator-master.yaml
  • ci-operator-4.10-configs configmap in namespace ci at cluster app.ci using the following files:
    • key openshift-machine-config-operator-release-4.10.yaml using file ci-operator/config/openshift/machine-config-operator/openshift-machine-config-operator-release-4.10.yaml
  • job-config-master-presubmits configmap in namespace ci at cluster app.ci using the following files:
    • key openshift-machine-config-operator-master-presubmits.yaml using file ci-operator/jobs/openshift/machine-config-operator/openshift-machine-config-operator-master-presubmits.yaml
  • job-config-4.10 configmap in namespace ci at cluster app.ci using the following files:
    • key openshift-machine-config-operator-release-4.10-presubmits.yaml using file ci-operator/jobs/openshift/machine-config-operator/openshift-machine-config-operator-release-4.10-presubmits.yaml
Details

In response to this:

Reverts #22940

Now that the PR to introduce the test is merged, we should make sure we are gathering the output on future PRs.

CC @kikisdeliveryservice @sinnykumari @yuqi-zhang

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@kikisdeliveryservice
Copy link
Contributor

kikisdeliveryservice commented Nov 11, 2021

Weird somehow my approval added a lgtm? Seems fine it merged, but v weird.

@sinnykumari
Copy link
Contributor

sinnykumari commented Nov 11, 2021

Weird somehow my approval added a lgtm? Seems fine it merged, but v weird.

I have noticed this as well lot of time for release repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yuqi-zhang yuqi-zhang Awaiting requested review from yuqi-zhang

1 more reviewer

@kikisdeliveryservice kikisdeliveryservice kikisdeliveryservice approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@kikisdeliveryservice kikisdeliveryservice

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@JoelSpeed @kikisdeliveryservice @sinnykumari @yuqi-zhang @openshift-merge-robot